Refactors how Slic keeps connections alive #3670
Conversation
src/IceRpc/Transports/Slic/Internal/SlicDuplexConnectionDecorator.cs
Outdated
Show resolved
Hide resolved
src/IceRpc/Transports/Slic/Internal/SlicDuplexConnectionDecorator.cs
Outdated
Show resolved
Hide resolved
src/IceRpc/Transports/Slic/Internal/SlicDuplexConnectionDecorator.cs
Outdated
Show resolved
Hide resolved
src/IceRpc/Transports/Slic/Internal/SlicDuplexConnectionDecorator.cs
Outdated
Show resolved
Hide resolved
| // Only client connections send ping frames when idle to keep the server connection alive. The server | ||
| // sends back a Pong frame in turn to keep alive the client connection. | ||
| _enableIdleTimeoutAndKeepAlive(idleTimeout, IsServer ? null : KeepAlive); | ||
| _duplexConnection.Enable(idleTimeout); |
There was a problem hiding this comment.
Would be better to keep IDuplexConnection and then do
if (_duplexConnection is SlicDuplexConnectionDecorator duplexConnectionDecoator)
{
duplexConnectionDecoator.Enable();
}
Then we can make SlicDuplexConnectionDecorator timers non-nullable, and keep a single constructor.
There was a problem hiding this comment.
I don't see how this would work.
The difficulty here is the idle timeout is negotiated during connection establishment. At construction time, we don't know if the negotiated idle timeout is good to be infinite or some other value.
There was a problem hiding this comment.
We can pass the idleTimeout I forget to add it there, and that is not the point. The point is to avoid creating this decorator for the Server connections.
There was a problem hiding this comment.
We need the decorator for server connections too. It implements the idle timer.
|
|
||
| void SendReadPing() | ||
| { | ||
| // ReadKeepAlive is no-op unless _pendingPongCount == 0 before the Increment below: we send a Ping only if |
There was a problem hiding this comment.
What is ReadKeepAlive there is no such operation?
There was a problem hiding this comment.
Fixed. It's the current local function, SendReadPing.
| void SendReadPing() | ||
| { | ||
| // ReadKeepAlive is no-op unless _pendingPongCount == 0 before the Increment below: we send a Ping only if | ||
| // there is no outstanding Pong. |
There was a problem hiding this comment.
is outstanding Pong. correct here?
There was a problem hiding this comment.
Replaced by pending Pong, like the field name.
This PR refactors how Slic keeps connections alive.
Fixes #3665.
With this new implementation, Slic keeps connection alive by sending Ping frames from client connections (like before).
The algorithm is now:
This PR also refactors the idle timeout decorator and associated tests. They are now 2 separate decorators, one for Ice and one for Slic. This PR didn't change the Ice logic.